BEEFY: add digest log on consensus reset #14765
Conversation
acatangiu
added
A0-please_review
| frame_support::weights::constants::RocksDbWeight::get().reads(1) | ||
| } | ||
|
|
||
| fn on_finalize(n: BlockNumberFor<T>) { |
There was a problem hiding this comment.
If I understand correctly this is needed because genesis_block can be a future block. But do we need to allow genesis_block to be a future block ? Could we just reset from the current block ?
There was a problem hiding this comment.
Resetting from current block could also work and would simplify this code - no hook required anymore, just emit digest log when the reset call happens, BUT in practice we'd lose control of selecting the actual BEEFY genesis block number..
Imagine we need to start/reset BEEFY on Kusama or Polkadot: we need to go through governance and propose a genesis block number. It cannot be in the past because past blocks are immutable and it's practically impossible to time the governance enactment block number with the desired beefy genesis block number.
So in order to control beefy-genesis block M, what we do is propose in governance a beefy-genesis future block M > governance motion/proposal lifetime (estimated at block N) so that:
- if motion fails, then it's noop
- if it succeeds, the call to set beefy-genesis block will be mined at some block
N'<=N<M- so it is valid and works
If we were to use current block then BEEFY genesis is simply the block when governance motion is enacted.
Regarding the need to control the number, I was talking to @andresilva that we'd likely want BEEFY genesis to be a session boundary block - although it's not really mandatory.
@bkchr wdyt? maybe we can use some other magic mechanism to delay a call to be executed on a predefined future block, and not have to rely on pallet-beefy to keep this logic?
There was a problem hiding this comment.
LGTM, but I have a couple of questions:
- if this digest is emitted during BEEFY restart (not during the start), is it more similar to the GRANDPA forced change or to a regular change? In other words - do we expect light clients to be able to swallow this block and keep syncing? Because e.g. forced change in GRANDPA is currently not supported by (at least our, bridge) light client? Do we then at least generate a mandatory BEEFY justification for such a block?
- if we are treating reset as the forced change, which is not supported by regular light clients, then maybe we could emit
ConsensusResetdigest at current block, but the digesst shall specify the block at which reset will actually happen? I.e.ConsensusReset(<future-block-number>, ValidatorSet<AuthorityId>). IIUC there's not much difference for a light client - it needs to know the BEEFY starting block to be able to start. And if this starting block would be the blockN', which in turn will point (using the digest) to the blockM.
But to me the current approach (emitting from block hooks) is ok too.
| impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> { | ||
| fn on_initialize(_n: BlockNumberFor<T>) -> Weight { | ||
| // weight used by `on_finalize()` hook | ||
| frame_support::weights::constants::RocksDbWeight::get().reads(1) |
There was a problem hiding this comment.
IIUC this isn't technically correct, because at BEEFY genesis block we'll be doing 3 db reads (Authorities and ValidatorSetId in addition to GenesisBlock). So probably it should be frame_support::weights::constants::RocksDbWeight::get().reads(if GenesisBlock::<T>::get() == Some(n) { 3 } else { 1 }). Don't think it is critical here, though :)
Deposit
ConsensusLogon BEEFY consensus reset.The log can be used by block import (or light clients) to detect any consensus reset without having to rely on state.